Adds linked editing for JSX tags #53284

iisaduan · 2023-03-16T06:58:49Z

Adds linked editing of JSX tags (mirror cursor)

typescript-bot · 2023-03-16T06:58:53Z

Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping @sheetalkamat, @mjbvz, @zkat, and @joj for you. Feel free to loop in other consumers/maintainers if necessary

typescript-bot · 2023-03-16T06:58:56Z

It looks like you've sent a pull request to update our 'lib' files. These files aren't meant to be edited by hand, as they consist of last-known good states of the compiler and are generated from 'src/lib' or possibly our lib generator. Unless this is necessary, consider closing the pull request and sending a separate PR to update 'src/lib' or https://github.com/microsoft/TypeScript-DOM-lib-generator

…mirror

tests/baselines/reference/api/typescript.d.ts

tests/baselines/reference/api/tsserverlibrary.d.ts

DanielRosenwasser

Looks like this is all coming together pretty nicely. I left a bit of feedback about

why things weren't working correctly with fragments,
some ideas for how fourslash testing can be made easier
some thoughts on the API (TextSpan vs. TextRange - maybe we revisit that?)
some style feedback

With regards to style, at some point we should consider getting an automatic formatter - but in the meantime, try to stay consistent with the rest of the codebase, and use the editor's built-in formatter as well.

src/harness/fourslashInterfaceImpl.ts

src/services/services.ts

tests/cases/fourslash/jsxTagLinkedEdit7.ts

src/services/services.ts

iisaduan · 2023-03-31T22:04:38Z

I've renamed the feature to linkedEditing..., to match the LSP and allow for potential non-JSX tag usage in the future.

DanielRosenwasser

I think you could use a few more edge-case tests. Tests where you're at the beginning of the file, end of the file, and where the code is not syntactically valid (e.g. a closing tag missing its tag name, or a closing fragment tag that "accidentally" has a tag name.

DanielRosenwasser · 2023-03-31T22:34:26Z

src/server/protocol.ts

@@ -23,6 +23,7 @@ import {

 export const enum CommandTypes {
    JsxClosingTag = "jsxClosingTag",
+    LinkedEditing = "LinkedEditing",


I'm guessing LSP uses this name, but every other command type in the TS Server protocol is mostly camelCase.

DanielRosenwasser · 2023-03-31T22:38:14Z

tests/baselines/reference/api/tsserverlibrary.d.ts

+            interface LinkedEditingRequest extends FileLocationRequest {
+                readonly command: CommandTypes.LinkedEditing;
+            }
+            interface LinkedEditingRanges {


It's odd that the ranges are a subfield. Looking at existing precedent, there's FileReferencesResponseBody, so maybe:

Suggested change

interface LinkedEditingRanges {

interface LinkedEditingRangesBody {

I had written the LinkedEditingRanges and LinkedEditingRangeResponse this way, as this is how they were set up in the stubs and LSP

I would also prefer if it matched the style we had in tsserver and was named Body; LSP may define it one way but I don't think we should be inconsistent to match them.

tests/cases/fourslash/fourslash.ts

src/harness/fourslashImpl.ts

src/harness/fourslashInterfaceImpl.ts

src/services/services.ts

mjbvz · 2023-04-03T18:16:30Z

src/services/services.ts

+
+            return {
+                ranges: [{ start: openTagStart, length: openTagEnd - openTagStart }, { start: closeTagStart, length: closeTagEnd - closeTagStart }],
+                wordPattern: openingTagText


This should actually be regex for valid tag names. Something like [a-z\._$][a-z0-9\._$]+ perhaps. You can also try omitting it if VS Code's word pattern is good

With the current implementation, VS Code stops linked editing as soon you type anything that is not the tag name

What happens if the returned ranges don't satisfy the regex? No linked cursors I assume?

I think the problem with that regex is that it expects at least two characters. Maybe you wanted:

[a-z\._$][a-z0-9\._$]*

?

Yes, but also try omitting it and seeing if VS Code does the right thing already. You should only need to return wordRange if the default behavior isn't correct

Yeah, in that case let's leave off wordPattern for now.

src/server/session.ts

typescript-bot · 2023-04-06T22:20:58Z

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

RyanCavanaugh · 2023-04-06T23:12:15Z

tests/cases/fourslash/fourslash.ts

@@ -759,6 +760,11 @@ declare namespace FourSlashInterface {
        generateReturnInDocTemplate?: boolean;
    }

+    type LinkedEditingInfo = {
+        readonly ranges : { start: number, length: number }[];


Suggested change

readonly ranges : { start: number, length: number }[];

readonly ranges: { start: number, length: number }[];

RyanCavanaugh · 2023-04-06T23:12:27Z

tests/cases/fourslash/fourslash.ts

@@ -759,6 +760,11 @@ declare namespace FourSlashInterface {
        generateReturnInDocTemplate?: boolean;
    }

+    type LinkedEditingInfo = {
+        readonly ranges : { start: number, length: number }[];
+        wordPattern? : string;


Suggested change

wordPattern? : string;

wordPattern?: string;

RyanCavanaugh · 2023-04-06T23:26:08Z

src/services/services.ts

+                    }
+                    return false;
+                });
+            if (!element || !(isJsxOpeningElement(element) || isJsxClosingElement(element))) return undefined;


Is (isJsxOpeningElement(element) || isJsxClosingElement(element)) possibly false given what the findAncestor call does? Fine to leave in just in case, but I'm curious if this/how this happens

Seems to now be mostly for the typeguard feature, as it seems other code I've added since covers these cases. Should I change it to an as JsxOpeningElement | JsxClosingElement | undefined assertion to the result of findAncestor?

jakebailey

Things seem good, but, I think I would like to see the protocol naming be consistent, as once we add it, we're not going to change it.

jakebailey · 2023-04-06T23:35:10Z

tests/baselines/reference/api/tsserverlibrary.d.ts

+            interface LinkedEditingRequest extends FileLocationRequest {
+                readonly command: CommandTypes.LinkedEditing;
+            }
+            interface LinkedEditingRanges {


I would also prefer if it matched the style we had in tsserver and was named Body; LSP may define it one way but I don't think we should be inconsistent to match them.

jakebailey · 2023-04-06T23:36:35Z

tests/cases/fourslash/linkedEditingJsxTag4.ts

+verify.linkedEditing( {
+    "0": linkedCursors,
+    "1": linkedCursors,
+    "2": undefined,


Maybe this is a lot of work, but maybe it's better to have a baseline for this instead, rather than all of these undefineds? I know that a lot of our current tests have this, but, I think we're trying to make them friendlier, e.g. @sandersn's new completion baselines.

jakebailey · 2023-04-06T23:37:25Z

src/services/services.ts

+            // determines if the cursor is in an element tag
+            const element = findAncestor(token.parent,
+                n => {
+                    if (!n.parent) return "quit";


Is this required? I think the only case where you won't have a parent is on SourceFile, in which case iteration is going to stop.

It caused an issue when you checked the kind of .parent, and .parent is undefined, but turns out, I was able to rewrite findAncestor so I didn't need to check .parent anymore

src/services/services.ts

typescript-bot · 2023-04-07T19:31:09Z

The TypeScript team hasn't accepted the linked issue #51832. If you can get it accepted, this PR will have a better chance of being reviewed.

iisaduan added 11 commits March 8, 2023 17:38

add tests and fourslash framework

409d949

cd

94c4b65

first implementation, all tests except 3 (namespace) passes

a5192e8

forgot to include into last commit

21aa9e7

passes tests!

398f673

change testing structure

2bafe8d

change testing structure

da0ddf1

add correct content into tests (and some more test cases)

9d7f32c

changes to getLinkedEdit.., saving some extra code in comments as well

1fff99d

remove comments, extra code, rename functions

0b0541d

remove commented code and todos, also renames functions

b70ee90

typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Mar 16, 2023

typescript-bot added the lib update PR modifies files in the `lib` folder label Mar 16, 2023

iisaduan added 2 commits March 16, 2023 00:13

revert changes to lib

25393f1

Merge branch 'mirror' of https://github.com/iisaduan/TypeScript into …

bb98df0

…mirror

mjbvz requested changes Mar 16, 2023

View reviewed changes

tests/baselines/reference/api/typescript.d.ts Outdated Show resolved Hide resolved

tests/baselines/reference/api/tsserverlibrary.d.ts Outdated Show resolved Hide resolved

iisaduan added 5 commits March 16, 2023 13:19

link everything together (fix session client)

6ba6321

missed renames

92875ce

linted

57ef9b8

update baselines

06afbbd

debugged whitespace issues

f4ec7bd

iisaduan changed the title ~~Adds linked editing for JSX tags, as specified in #51832~~ Adds linked editing for JSX tags Mar 17, 2023

DanielRosenwasser requested changes Mar 17, 2023

View reviewed changes

iisaduan added 5 commits March 20, 2023 14:11

fixed some whitespace in code

803b292

adds better behavior for whitespace and trivia in fragments

34f5e40

refactor getJsxLinkedEditAtPosition

03fa5bb

change type to TextSpan

4faee99

updated tests to have textspan

6a1d3dd

iisaduan added 4 commits March 30, 2023 23:29

update protocol, refactor services

be34a1e

cleaned up tests

7f2f885

rename to match lsp

5164495

rename tests

a3bdf2e

iisaduan requested review from DanielRosenwasser and mjbvz March 31, 2023 22:04

DanielRosenwasser requested changes Mar 31, 2023

View reviewed changes

more-readable code/comments in getLinkedEditingAtPosition

228e0ae

mjbvz reviewed Apr 3, 2023

View reviewed changes

DanielRosenwasser reviewed Apr 3, 2023

View reviewed changes

src/server/session.ts Outdated Show resolved Hide resolved

iisaduan added 2 commits April 4, 2023 15:29

add more tests + add range to function names

32e43c8

more tests

da4f1cd

iisaduan marked this pull request as ready for review April 6, 2023 22:20

RyanCavanaugh reviewed Apr 6, 2023

View reviewed changes

RyanCavanaugh approved these changes Apr 6, 2023

View reviewed changes

jakebailey reviewed Apr 6, 2023

View reviewed changes

update Body, simplify call to findAncestor

db67aee

jakebailey approved these changes Apr 7, 2023

View reviewed changes

typescript-bot assigned iisaduan Apr 7, 2023

typescript-bot added For Milestone Bug PRs that fix a bug with a specific milestone and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Apr 7, 2023

iisaduan merged commit d4c48e1 into microsoft:main Apr 7, 2023

qiqiboy mentioned this pull request Apr 10, 2023

[feature request]Add linkedEditing LSP feature neoclide/coc-tsserver#423

Closed

eps1lon mentioned this pull request Apr 15, 2023

[test] Intrinsics with colon no longer type-checked #53784

Closed

iisaduan mentioned this pull request Apr 20, 2023

Linked editing disconnects on valid tag names with non alphanumeric characters #53936

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds linked editing for JSX tags #53284

Adds linked editing for JSX tags #53284

iisaduan commented Mar 16, 2023 •

edited

Loading

typescript-bot commented Mar 16, 2023

typescript-bot commented Mar 16, 2023

DanielRosenwasser left a comment

iisaduan commented Mar 31, 2023

DanielRosenwasser left a comment

DanielRosenwasser Mar 31, 2023

DanielRosenwasser Mar 31, 2023

iisaduan Apr 3, 2023

jakebailey Apr 6, 2023

mjbvz Apr 3, 2023

DanielRosenwasser Apr 3, 2023

mjbvz Apr 3, 2023

DanielRosenwasser Apr 3, 2023

typescript-bot commented Apr 6, 2023

RyanCavanaugh Apr 6, 2023

RyanCavanaugh Apr 6, 2023

RyanCavanaugh Apr 6, 2023

iisaduan Apr 7, 2023

jakebailey left a comment •

edited

Loading

jakebailey Apr 6, 2023

jakebailey Apr 6, 2023

jakebailey Apr 6, 2023

iisaduan Apr 7, 2023

typescript-bot commented Apr 7, 2023

	interface LinkedEditingRanges {
	interface LinkedEditingRangesBody {

	readonly ranges : { start: number, length: number }[];
	readonly ranges: { start: number, length: number }[];

Adds linked editing for JSX tags #53284

Adds linked editing for JSX tags #53284

Conversation

iisaduan commented Mar 16, 2023 • edited Loading

typescript-bot commented Mar 16, 2023

typescript-bot commented Mar 16, 2023

DanielRosenwasser left a comment

Choose a reason for hiding this comment

iisaduan commented Mar 31, 2023

DanielRosenwasser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

typescript-bot commented Apr 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakebailey left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

typescript-bot commented Apr 7, 2023

iisaduan commented Mar 16, 2023 •

edited

Loading

jakebailey left a comment •

edited

Loading